feat: basic table scan planning #112

gty404 · 2025-05-27T06:46:04Z

Introducing basic scan table data interface

src/iceberg/result.h

src/iceberg/table_scan.h

src/iceberg/table_scan.cc

src/iceberg/type_fwd.h

src/iceberg/table_scan.h

src/iceberg/type_fwd.h

src/iceberg/table_scan.h

src/iceberg/table_scan.cc

Co-authored-by: Gang Wu <[email protected]>

src/iceberg/snapshot.h

src/iceberg/table_scan.h

src/iceberg/manifest_reader.h

src/iceberg/snapshot.h

src/iceberg/table_scan.h

src/iceberg/table_scan.cc

src/iceberg/manifest_reader.h

src/iceberg/table_scan.h

wgtmac · 2025-07-01T10:02:25Z

src/iceberg/table_scan.h

+};
+
+/// \brief Represents a task to scan a portion of a data file.
+class ICEBERG_EXPORT FileScanTask : public ScanTask {


Some thoughts about FileScanTask:

Should we remove ScanTask abstraction above? If we remove the abstraction, we can directly use aggregate initialization to create a task. Otherwise we may need to expand the constructor every time a new parameter is required.

If we do (1) above, is it possible also to make it a simple struct by removing all functions (as they are all trivial accessors).

Should we add fields (a.k.a. spec and partition_value) from Java PartitionScanTask to support partitioning? We can add them later but a TODO comment is desirable.

Should we combine start and length, and wrap them by std::optional? I believe they are not required at all times.

I initially expected it to just be a struct, but since the previous comments suggested doing an abstraction, I referred to the design in iceberg-java/iceberg-python.

Partition spec and value can be obtained from DataFile and Snapshot, and we can add these interfaces when needed for subsequent PR

Sure, I will modify it to optional, thanks.

Partition spec and value can be obtained from DataFile and Snapshot

That's a good point

src/iceberg/table_scan.h

wgtmac · 2025-07-01T14:07:43Z

src/iceberg/table_scan.cc

+        data_entry.sequence_number.value_or(TableMetadata::kInitialSequenceNumber);
+    for (auto it = sequence_index.lower_bound(data_sequence_number);
+         it != sequence_index.end(); ++it) {
+      // Additional filtering logic here


What is the additional filtering logic? Did you mean to further check if the delete files can be filtered?

DataFiles only need to retain DeleteFiles with a sequence greater than their own?

This differs per equality and positional deletes. I think there is a pretty good overview here: https://iceberg.apache.org/spec/#scan-planning

src/iceberg/table_scan.cc

wgtmac · 2025-07-01T14:15:16Z

src/iceberg/table_scan.cc

+  return sizeInBytes;
+}
+
+int32_t FileScanTask::files_count() const {


I'm not sure if we need to rename it to FilesCount(). @lidavidm suggestion?

wgtmac · 2025-07-03T01:26:24Z

src/iceberg/table_scan.h

+};
+
+/// \brief Represents a task to scan a portion of a data file.
+class ICEBERG_EXPORT FileScanTask : public ScanTask {


Partition spec and value can be obtained from DataFile and Snapshot

That's a good point

src/iceberg/table_scan.h

wgtmac · 2025-07-03T02:08:34Z

src/iceberg/table_scan.h

+};
+
+/// \brief A scan that reads data files and applies delete files to filter rows.
+class ICEBERG_EXPORT DataScan : public TableScan {


I'm a little bit confused about the name of Scan and ScanTask across different implementations. Should this be DataTableScan which produces FileScanTask? For DataScan, I think it should produce a group of DataTask which contains rows of FileScanTask.

Simply put:
Scan -> ScanTask
TableScan -> FileScanTask
DataScan -> DataTask

DataScan inherits TableScan inherits Scan
DataTask inherits FileScanTask inherits ScanTask

WDYT? @gty404 @Fokko

BTW, I think we can constantly evolve this design because APIs can be unstable before the 1.0.0 release.

At PyIceberg we (tried to) copied the Java structure, but in the end I think it is too much OOP for Python. Maybe good to start small in C++ as well. While we can change APIs until 1.0.0, I think it is important to get this one right pretty early on, since this is the main integration point for query engines.

src/iceberg/table_scan.cc

src/iceberg/manifest_reader.h

src/iceberg/table_scan.cc

mapleFU · 2025-07-03T14:12:55Z

src/iceberg/table_scan.cc

+    for (auto it = sequence_index.lower_bound(data_sequence_number);
+         it != sequence_index.end(); ++it) {


Is this incorrect? Since it find the lowerbound and traverse all the sequence numbers above data_sequence_number

Yes, the meaning here is to find all DeleteFiles corresponding to this DataFile. Only those with a sequence number higher than the DataFile need to be read.

Higher or equal for positional deletes: https://iceberg.apache.org/spec/#scan-planning

src/iceberg/table_scan.cc

wgtmac

I just have a comment w.r.t. the table scan name. Elsewhere LGTM.

src/iceberg/table_scan.h

src/iceberg/table_scan.cc

Fokko · 2025-07-04T14:05:27Z

src/iceberg/table.cc

@@ -107,4 +108,8 @@ const std::vector<SnapshotLogEntry>& Table::history() const {

 const std::shared_ptr<FileIO>& Table::io() const { return io_; }

+std::unique_ptr<TableScanBuilder> Table::NewScan() const {
+  return std::make_unique<TableScanBuilder>(metadata_, io_);


How about passing in the Table instead? That has all the metadata, and also the io

If I pass "Table" to the "TableScanBuilder," I cannot pass it further to "DataTableScan," as "Table" can only be passed by reference to the "TableScanBuilder."

@Fokko There are some (outdated) comments on this: #112 (comment)

src/iceberg/table_scan.cc

Fokko · 2025-07-04T14:19:56Z

src/iceberg/table_scan.cc

+        data_entry.sequence_number.value_or(TableMetadata::kInitialSequenceNumber);
+    for (auto it = sequence_index.lower_bound(data_sequence_number);
+         it != sequence_index.end(); ++it) {
+      // Additional filtering logic here


This differs per equality and positional deletes. I think there is a pretty good overview here: https://iceberg.apache.org/spec/#scan-planning

Fokko · 2025-07-04T14:30:18Z

src/iceberg/table_scan.cc

+    for (auto it = sequence_index.lower_bound(data_sequence_number);
+         it != sequence_index.end(); ++it) {


Higher or equal for positional deletes: https://iceberg.apache.org/spec/#scan-planning

src/iceberg/table_scan.cc

wgtmac

I just reviewed it for another pass and added some nits (we can improve them in followup PRs). Thanks for working on it!

wgtmac · 2025-07-13T14:55:58Z

src/iceberg/table_scan.cc

+FileScanTask::FileScanTask(std::shared_ptr<DataFile> file)
+    : data_file_(std::move(file)) {}


Suggested change

FileScanTask::FileScanTask(std::shared_ptr<DataFile> file)

: data_file_(std::move(file)) {}

FileScanTask::FileScanTask(std::shared_ptr<DataFile> data_file)

: data_file_(std::move(data_file)) {}

nit: I think eventually we need to rename it to data_file once we will add delete files.

wgtmac · 2025-07-13T15:16:51Z

src/iceberg/table_scan.cc

+
+TableScanBuilder& TableScanBuilder::WithColumnNames(
+    std::vector<std::string> column_names) {
+  column_names_ = std::move(column_names);


nit: make sure context_.projected_schema is not set.

wgtmac · 2025-07-13T15:17:05Z

src/iceberg/table_scan.cc

+}
+
+TableScanBuilder& TableScanBuilder::WithProjectedSchema(std::shared_ptr<Schema> schema) {
+  context_.projected_schema = std::move(schema);


nit: make sure column_names_ is not set.

In a conflict situation, it's not very convenient to throw an exception here, I want to put the check in the build.

wgtmac · 2025-07-13T15:21:05Z

src/iceberg/table_scan.cc

+    return InvalidArgument("No snapshot ID specified for table {}",
+                           table_metadata->table_uuid);
+  }
+  auto iter = std::ranges::find_if(


nit: add Result<std::shared_ptr<Snapshot>> TableMetadata::Snapshot(int64_t snapshot_id) const and move the logic below to it.

wgtmac · 2025-07-13T15:22:25Z

src/iceberg/table_scan.cc

+    }
+
+    const auto& schemas = table_metadata->schemas;
+    const auto it = std::ranges::find_if(schemas, [id = *schema_id](const auto& schema) {


ditto, we can add TableMetadata::Schema(int64_t schema_id) for this.

Xuanwo

Thank you for working on this. I'm glad we've reached a consensus. Let's move forward!

gty404 added 4 commits May 27, 2025 14:43

feat: basic table scan planning

e971cc4

fix cpp lint

5fc6971

fix build fail on windows

6a2cb74

fix lint

d71c26a

lidavidm reviewed May 27, 2025

View reviewed changes

src/iceberg/result.h Outdated Show resolved Hide resolved

src/iceberg/table_scan.h Outdated Show resolved Hide resolved

gty404 added 2 commits May 27, 2025 16:18

fix some comments

c6c1a1f

fix clang format

cd07a0c

lidavidm reviewed May 28, 2025

View reviewed changes

src/iceberg/table_scan.h Outdated Show resolved Hide resolved

src/iceberg/table_scan.h Outdated Show resolved Hide resolved

src/iceberg/table_scan.cc Outdated Show resolved Hide resolved

src/iceberg/table_scan.cc Outdated Show resolved Hide resolved

wgtmac reviewed May 28, 2025

View reviewed changes

gty404 added 2 commits May 29, 2025 10:07

fix some comments

b7becc2

Merge branch 'main' into table-scan

abfdfcd

wgtmac reviewed May 30, 2025

View reviewed changes

yingcai-cy reviewed Jun 5, 2025

View reviewed changes

src/iceberg/table_scan.h Outdated Show resolved Hide resolved

src/iceberg/table_scan.cc Outdated Show resolved Hide resolved

src/iceberg/table_scan.cc Outdated Show resolved Hide resolved

gty404 and others added 3 commits June 14, 2025 14:08

Update src/iceberg/table_scan.h

28043b1

Co-authored-by: Gang Wu <[email protected]>

Update src/iceberg/table_scan.h

fa25891

Co-authored-by: Gang Wu <[email protected]>

Merge branch 'main' into table-scan

428651f

gty404 force-pushed the table-scan branch from 6cbd651 to 428651f Compare June 14, 2025 06:42

gty404 added 6 commits June 14, 2025 14:49

fix comments

812a545

Merge branch 'main' into table-scan

0f79c7c

Abstract TableScan and ScanTask

85802e9

fix lint

c7621b3

fix lint

e1267fc

fix lint

5248e22

zhjwpku reviewed Jun 28, 2025

View reviewed changes

src/iceberg/snapshot.h Outdated Show resolved Hide resolved

src/iceberg/table_scan.h Outdated Show resolved Hide resolved

wgtmac reviewed Jun 29, 2025

View reviewed changes

lishuxu reviewed Jun 29, 2025

View reviewed changes

src/iceberg/table_scan.h Outdated Show resolved Hide resolved

Merge branch 'main' into table-scan

368e268

lishuxu reviewed Jun 30, 2025

View reviewed changes

src/iceberg/table_scan.cc Outdated Show resolved Hide resolved

gty404 added 2 commits June 30, 2025 10:26

resolve some comments

29e8865

remove Snapshot::kInitialSequenceNumber

ae560f3

wgtmac reviewed Jul 1, 2025

View reviewed changes

src/iceberg/table_scan.h Outdated Show resolved Hide resolved

src/iceberg/table_scan.h Show resolved Hide resolved

wgtmac reviewed Jul 1, 2025

View reviewed changes

resolve some comments

0ff952b

wgtmac reviewed Jul 3, 2025

View reviewed changes

Fokko reviewed Jul 3, 2025

View reviewed changes

src/iceberg/table_scan.cc Outdated Show resolved Hide resolved

resolve some comments

1b5d123

mapleFU reviewed Jul 3, 2025

View reviewed changes

resolve some comments

3dc2b38

wgtmac approved these changes Jul 4, 2025

View reviewed changes

src/iceberg/table_scan.h Outdated Show resolved Hide resolved

resolve comments

702d0f4

lishuxu approved these changes Jul 4, 2025

View reviewed changes

mapleFU approved these changes Jul 4, 2025

View reviewed changes

Fokko reviewed Jul 4, 2025

View reviewed changes

gty404 added 3 commits July 7, 2025 09:26

resolve some comments

4342887

Trigger CI

0d9b89e

Trigger CI

e4af0e7

zhjwpku reviewed Jul 8, 2025

View reviewed changes

src/iceberg/table_scan.cc Outdated Show resolved Hide resolved

src/iceberg/table_scan.cc Outdated Show resolved Hide resolved

resolve some comments

8705a82

zhjwpku approved these changes Jul 8, 2025

View reviewed changes

wgtmac approved these changes Jul 8, 2025

View reviewed changes

wgtmac approved these changes Jul 13, 2025

View reviewed changes

Xuanwo approved these changes Jul 14, 2025

View reviewed changes

Xuanwo merged commit ef4c124 into apache:main Jul 14, 2025
7 checks passed

gty404 deleted the table-scan branch July 14, 2025 14:09

		for (auto it = sequence_index.lower_bound(data_sequence_number);
		it != sequence_index.end(); ++it) {

		FileScanTask::FileScanTask(std::shared_ptr<DataFile> file)
		: data_file_(std::move(file)) {}

feat: basic table scan planning #112

feat: basic table scan planning #112

Uh oh!

Conversation

gty404 commented May 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wgtmac Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac Jul 3, 2025 •

edited

Loading